-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add HIC Binding Models #121
Conversation
94c2622
to
6706fc8
Compare
Thanks for your help! |
Thanks a lot! I can update it myself. In case you're interested in how to push to this repo, you'll have to add our Github repository as remote repository. To add,
and ours could be
To check existing remote locations, use
Finally, when you push, you can now specify the location.
|
6706fc8
to
679052b
Compare
679052b
to
f30d21a
Compare
Hey, I added some tests to both HIC models using the parameters you provided and adding other components. Unfortunately, there still seem to be some issues. Here is a part of the output:
I will double check whether I made some mistake while setting up the test or if this is really an issue with the implementation (of the jacobian). In any way, I wanted to push what I've got so far. |
f30d21a
to
9cbc28c
Compare
9cbc28c
to
fab56d3
Compare
The pipeline failed with:
I assume, to fix, we simply need to add The dispatch pipeline always fails. It should probably be revised at some point. |
You have a syntax error in your yaml file. See: https://github.com/modsim/CADET/actions/runs/4510620259/workflow |
@ronald-jaepel ready to merge? |
@schmoelder I think yes |
Cool! Congrats! |
Not yet but I'll read into it. |
Read into it. Should all changes be squashed into a single commit or one per Isotherm or any other suggestions? |
haha, you're a machine! :) Btw, I can highly recommend checking out Lazygit (kudos to @jayghoshter for introducing it to me!).
Since they are very related, I think it's fine to put them all into a single commit. But I'd make sure to leave the CI fix on a separate commit. Note that rebasing "rewrites" history. So it's very powerful but can also be dangerous. You will also need to force push. And that's something we should never do on |
54c00ef
to
12f4bd7
Compare
38fd880
to
6e4d664
Compare
Adds the possibility to assign a default value to a parameter in the template used to generate the actual binding model and reaction model code. If the underlying parameter class supports it, the provided default value is assigned to the parameter if it is not found in the ParameterProvider.
Fixes the literature reference in the code and the docs of the HIC binding models. Also removes unnecessary casts and promotes the constant water activity to a (constant) parameter with a default value.
6e4d664
to
48af85a
Compare
This PR combines #116 and #118 .
To do
@ronald-jaepel provided us with some typical test parameters for HICWHS:
And for HICCWA:
@lieres could you have a look at the naming of the isotherms? We decided not to call them "Wang" and "Jäpel" isotherms, instead they are now called "HIC - Water on Hydrophobic Surfaces" and "HIC - Constant Water Activity".